Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Jlc/experience relationships attributes #64

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Jlc/experience relationships attributes #64

merged 1 commit into from
Jul 10, 2023

Conversation

johanseto
Copy link
Collaborator

Description

This change is applied to course-experience api.
The relations file inherits from json api relation but add some extra fields
in the attributes of the relationship model.
The base json api only adds id and type, with this change you can add extra model
attributes.
https://github.com/django-json-api/django-rest-framework-json-api/blob/main/rest_framework_json_api/relations.py#L255

The attributes field was selected due to the recommendation of jsonapi.
https://jsonapi.org/format/#document-resource-objects

Testing instructions

Clone this repo and look al the course experience API endpoints.
Know they have extra data to each relationship.

{
    "like/units": "/eox-nelp/api/experience/v1/like/units/",
    "report/units": "/eox-nelp/api/experience/v1/report/units/",
    "like/courses": "/eox-nelp/api/experience/v1/like/courses/",
    "report/courses": "/eox-nelp/api/experience/v1/report/courses/",
    "feedback/courses": "/eox-nelp/api/experience/v1/feedback/courses/",
    "feedback/public/courses": "/eox-nelp/api/experience/v1/feedback/public/courses/"
}

Before

No extra field data to the relationships. Only id and type.
Peek 2023-07-06 15-57

After

Configurable attributes field with model relation fields.
Peek 2023-07-06 15-58

Additional information

jira story:
https://edunext.atlassian.net/jira/software/c/projects/FUTUREX/boards/36?modal=detail&selectedIssue=FUTUREX-459

next steps

Remove username for the main attributes field of each experience APIview because with this change the relationship of the author already has it.

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

@github-actions github-actions bot added the size/m m lines label label Jul 6, 2023
andrey-canon
andrey-canon requested changes Jul 6, 2023
View reviewed changes
eox_nelp/course_experience/api/v1/relations.py Outdated Show resolved Hide resolved
eox_nelp/course_experience/api/v1/relations.py Outdated Show resolved Hide resolved
eox_nelp/course_experience/api/v1/relations.py Outdated Show resolved Hide resolved
eox_nelp/course_experience/api/v1/relations.py Outdated Show resolved Hide resolved
eox_nelp/course_experience/api/v1/serializers.py Outdated
Comment on lines 16 to 17
COURSE_OVERVIEW_EXTRA_ATTRIBUTES = ["display_name"]
USER_EXTRA_ATTRIBUTES = ["first_name", "last_name", "fullname", "username"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a setting, so we can add an extra attribute whenever we want

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be another PR?

eox_nelp/course_experience/api/v1/relations.py Outdated Show resolved Hide resolved
eox_nelp/course_experience/api/v1/tests/mixins_helpers.py Show resolved Hide resolved
eox_nelp/course_experience/api/v1/views.py Outdated
@@ -228,13 +228,15 @@ class LikeDislikeUnitExperienceView(UnitExperienceView):
"author": {
"data": {
"type": "User",
"id": "7"
"id": "7",
"attributes": {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why empty if you already added USER_EXTRA_ATTRIBUTES.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it could be variable ?

eox_nelp/course_experience/api/v1/views.py Outdated Show resolved Hide resolved
eox_nelp/course_experience/api/v1/tests/test_views.py
}
}
}
"relationships": self.make_relationships_data()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are you testing this new behavior ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eox-nelp/eox_nelp/course_experience/api/v1/tests/mixins_helpers.py

Line 233 in 9bd8f11

self.assertEqual(response.json()["data"]["relationships"], expected_data["data"]["relationships"])

eox-nelp/eox_nelp/course_experience/api/v1/tests/mixins_helpers.py

Line 336 in 9bd8f11

self.assertEqual(response.json(), self.base_data)

@johanseto johanseto force-pushed the jlc/experience-relationships-attributes branch 3 times, most recently from b368ae3 to 9bd8f11 Compare July 7, 2023 23:35
andrey-canon
andrey-canon reviewed Jul 8, 2023
View reviewed changes
eox_nelp/course_experience/api/v1/serializers.py Outdated
Also its is possible to check nested fields if you declarate the field of instance separated by `__`
eg:
{
"field_level1__field_level2": "key_name"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you interchange that, I mean {"key_name": "field_level1__field_level2"}, I don't have an specific use of that but if you interchange that it's possible to have different keys with same value

{
  "key_name": "field_level1__field_level2",
  "key_2": "field_level1__field_level2"
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e371330

eox_nelp/course_experience/api/v1/tests/mixins_helpers.py Outdated
"data": {
"type": "CourseOverview",
"id": f"{self.my_course.id}",
"attributes": map_attributes_from_instance_to_dict(self.user, COURSE_OVERVIEW_EXTRA_FIELD_MAPPING),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.user ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

de7a82f

eox_nelp/course_experience/api/v1/tests/mixins_helpers.py Outdated
"data": {
"type": "User",
"id": f"{self.user.id}",
"attributes": map_attributes_from_instance_to_dict(self.user, USER_EXTRA_FIELD_MAPPING),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are not you using the method get_course_extra_attributes and get_user_extra_attributes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I passed, maybe I was more focused on the map_dictionary function. Is better what you suggest.
de7a82f

@johanseto johanseto force-pushed the jlc/experience-relationships-attributes branch from 990ab6d to de7a82f Compare July 10, 2023 19:52
@johanseto johanseto force-pushed the jlc/experience-relationships-attributes branch from de7a82f to 396e258 Compare July 10, 2023 21:48
@johanseto
feat: add functions to add extra fields
79c1152 
refactor: change relationship expected data to tests

This allow to match the expected new fields in the attributes relationship
to the tests.

style: clean pylint and add docstring

refactor: change configuration for mapping

style: pr recommendation for tests
@johanseto johanseto force-pushed the jlc/experience-relationships-attributes branch from 396e258 to 79c1152 Compare July 10, 2023 21:49
@johanseto johanseto merged commit e75982e into master Jul 10, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrey-canon andrey-canon andrey-canon approved these changes

Assignees

@andrey-canon andrey-canon

Labels
course_experience size/m m lines label test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@johanseto @andrey-canon